Conversation
shs96c
reviewed
Jun 4, 2026
shs96c
left a comment
shs96c
left a comment
Member
There was a problem hiding this comment.
Where we host the image is the only thing I'm seeing here I'm not comfortable with
| "/usr/include/c++/11/backward", | ||
| ], | ||
| cxx_flags = ["-std=c++0x"], | ||
| cxx_flags = ["-std=c++17"], |
Member
There was a problem hiding this comment.
If we're setting this here, I think we can remove the equivalent entries in the .bazelrc. Can be done in a follow up PR.
Member
Author
There was a problem hiding this comment.
Hmm, isn't this file only enabled on RBE, while .bazelrc is enabled for everything?
Contributor
Review Summary by QodoUpdate Bazel C++ toolchain to Bazel 9 with C++20 modules and Ubuntu 22
WalkthroughsDescription• Update Bazel C++ toolchain configuration to Bazel 9 with rules_cc migration • Add support for C++20 modules and modern compiler features • Update RBE container image to Ubuntu 22 with GCC 11 • Switch from JRuby to MRI Ruby for RBE builds • Add sanitizer features (ASAN, TSAN, UBSAN) and improved macOS support Diagramflowchart LR
A["Bazel 9 Migration"] --> B["rules_cc Updates"]
B --> C["C++20 Module Support"]
B --> D["Toolchain Config Refactor"]
E["Ubuntu 22 RBE Image"] --> F["GCC 11 Compiler"]
F --> G["Updated Include Paths"]
H["MRI Ruby Support"] --> I["Remove JRuby Dependency"]
D --> J["New Features: Sanitizers, macOS Support"]
File Changes1. common/remote-build/cc/cc_toolchain_config.bzl
|
Contributor
Code Review by Qodo
1. Build setting in deps
|
Contributor
|
Code review by qodo was updated up to the latest commit 7adbc4d |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
19
to
+31
| set -eu | ||
|
|
||
| OUTPUT= | ||
|
|
||
| parse_option() { | ||
| opt=$1 | ||
| if [ "$OUTPUT" = "1" ]; then | ||
| OUTPUT=$opt | ||
| elif [ "$opt" = "-o" ]; then | ||
| # output is coming | ||
| OUTPUT=1 | ||
| fi | ||
| } |
Contributor
There was a problem hiding this comment.
1. parse_option can exit wrapper 📘 Rule violation ☼ Reliability
cc_wrapper.sh enables set -e but parse_option() can return a non-zero status for most arguments, which can prematurely terminate the wrapper and break header parsing/compilation. This violates the expectation that CI/automation scripts behave fail-safely with robust command handling.
Agent Prompt
## Issue description
`common/remote-build/cc/cc_wrapper.sh` uses `set -eu` and calls `parse_option` as a simple command. In POSIX `sh`, a function’s return status is the status of its last executed command; here that can be the `[ ... ]` test, which returns `1` for most args. With `set -e`, that non-zero return can abort the wrapper.
## Issue Context
This wrapper runs in build actions; unexpected early exits can cause flaky or consistently failing builds in RBE/CI.
## Fix Focus Areas
- common/remote-build/cc/cc_wrapper.sh[19-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+42
| cc_library(name = "empty_lib") | ||
|
|
||
| # Label flag for extra libraries to be linked into every binary. | ||
| label_flag( | ||
| name = "link_extra_libs", | ||
| build_setting_default = ":empty_lib", | ||
| ) | ||
|
|
||
| # The final extra library to be linked into every binary target. This collects | ||
| # the above flag, but may also include more libraries depending on config. | ||
| cc_library( | ||
| name = "link_extra_lib", | ||
| deps = [ | ||
| ":link_extra_libs", | ||
| ], | ||
| ) |
Contributor
There was a problem hiding this comment.
3. Build setting in deps 🐞 Bug ≡ Correctness
//common/remote-build/cc:link_extra_lib is a cc_library that lists :link_extra_libs in deps, but :link_extra_libs is a label_flag build setting rather than a C/C++ provider. This is likely to fail analysis when building the generated toolchain repo (e.g., during `bazel build @local_config_cc//...`).
Agent Prompt
## Issue description
`cc_library(name = "link_extra_lib")` depends directly on `:link_extra_libs`, but `:link_extra_libs` is a `label_flag` build setting (not a C/C++ library target). This wiring is very likely to break Bazel analysis for targets that evaluate `//common/remote-build/cc:link_extra_lib`.
## Issue Context
This directory is generated/copied as part of toolchain regeneration and is intended to be buildable.
## Fix Focus Areas
- common/remote-build/cc/BUILD[27-42]
- scripts/remote-image/create-cc-toolchain-within-image.sh[14-27]
## What to change
- Make `//common/remote-build/cc:link_extra_lib` depend only on real `cc_library` targets (e.g., `:empty_lib`) OR remove `link_extra_lib` entirely if unused.
- If the intent is to make a build-setting-controlled library available to toolchain logic, implement an explicit Starlark rule/macro that reads the build setting value and forwards the referenced target’s `CcInfo` (instead of placing the `label_flag` target itself in `deps`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 4cba6dc |
Contributor
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the Ruby ruleset for Bazel, which now supports MRI (CRuby) toolchains on RBE. We no longer need to use JRuby for the RBE job and can stick to the default Ruby interpreter.